Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add extensions support for Tor OTR profile #7304

Merged
merged 7 commits into from
Dec 15, 2020
Merged

Add extensions support for Tor OTR profile #7304

merged 7 commits into from
Dec 15, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Dec 1, 2020 •
edited
Loading

This PR

  1. Leverage "Allow in private" to enable a extension in Tor
  2. When a extension is running in spanning mode, warn users that network requests made by the extension will not go through Tor
    (https://developers.chrome.com/extensions/manifest/incognito)

Security review submitted: https://github.com/brave/security/issues/258
Resolves brave/brave-browser#2761

We can only merge this PR after master upgrade to 88.0.4284.0, otherwise Tor OTR profile won't be destroyed even there is no Tor window opened and that will leave zombie private extensions.
This is the change we need
https://chromium-review.googlesource.com/c/chromium/src/+/2434925/6/chrome/browser/ui/browser.cc#605
Master has already upgraded to C88

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Enable extension in Tor

  1. Install any extension
  2. Visit brave://extensions/ and click Details of the extension
  3. Turn on Allow in private
  4. The extension should be accessible in Tor window

Spanning vs Split

  1. Extensions run in spanning mode by default so go to the extension page of Enable extension in Tor
  2. We should see warning text like this Screen Shot 2020-12-14 at 14 48 35
  3. Go to user profile folder, find the extension folder with the id seen in extension page
  4. Copy the folder to somewhere
  5. Open the copied folder and find manifest.json
  6. Edit manifest.json and add "incognito": "split", and save.
  7. Visit brave://extensions/ and turn on developer mode and click Load unpacked
  8. Choose the copied folder
  9. It should load the copied version of the extension
  10. Go to the detail of the extension, we should see warning text like Screen Shot 2020-12-14 at 14 53 39

@darkdh darkdh self-assigned this Dec 1, 2020
@darkdh darkdh force-pushed the tor-extensions-support branch 3 times, most recently from 397c56f to fc228ee Compare December 3, 2020 21:45
@darkdh darkdh marked this pull request as ready for review December 3, 2020 22:24
@darkdh darkdh requested a review from a team as a code owner December 3, 2020 22:24
@darkdh darkdh force-pushed the tor-extensions-support branch 5 times, most recently from 5c9109f to 5300016 Compare December 8, 2020 22:30
petemill
petemill reviewed Dec 9, 2020
View reviewed changes
patches/chrome-browser-resources-extensions-detail_view.html.patch Outdated
</div>
</extensions-toggle-row>
- </template>
+ <extensions-brave-tor-toggle-row data="[[data]]" delegate="[[delegate]]">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested in DM, I suggest injecting the HTML and JS for the new toggle directly to the chromium component via RegisterPolymerTemplateModifications and RegisterPolymerComponentBehaviors, so that you can keep the directly-applied styles for the row instead of having to copy/paste. You also wouldn't need this patch or the .js file patch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@darkdh darkdh force-pushed the tor-extensions-support branch 8 times, most recently from bbdc3ee to cbbc245 Compare December 11, 2020 01:10
bridiver
bridiver reviewed Dec 11, 2020
View reviewed changes
patches/chrome-browser-resources-extensions-service.js.patch Outdated
incognitoAccess: isAllowedIncognito,
});
}
+ setItemAllowedTor(id, isAllowedTor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check with @petemill on this? At the very least it can be compressed to one line

darkdh and others added 5 commits December 14, 2020 14:59
@darkdh
lint fix
4fcd0d4
@darkdh
Enabling extensions in Tor through "Allow in private" and warn users
4bac061 
that network requests will not go through Tor if the extension is running in
spanning mode
@darkdh darkdh force-pushed the tor-extensions-support branch from 9304c6d to 4bac061 Compare December 14, 2020 22:59
@darkdh
Copy link
Member Author

darkdh commented Dec 15, 2020

Known CI issue

16:22:24  Failing tests:
16:22:24  	TestTargetName:
16:22:24  		-[AdsWrapperTest testPreferencePersistance]
16:22:24  
16:22:24  ** TEST EXECUTE FAILED **

https://ci.brave.com/job/pr-brave-browser-tor-extensions-support-ios/20/execution/node/216/log/

@darkdh darkdh merged commit 8c0a444 into master Dec 15, 2020
@darkdh darkdh deleted the tor-extensions-support branch December 15, 2020 20:56
@darkdh darkdh added this to the 1.20.x - Nightly milestone Dec 15, 2020
@stephendonner stephendonner mentioned this pull request Jan 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petemill petemill petemill left review comments

@bridiver bridiver bridiver approved these changes

@mariospr mariospr Awaiting requested review from mariospr

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@darkdh darkdh

Labels
None yet
Projects
None yet
Milestone
1.20.x - Release
Development

Successfully merging this pull request may close these issues.

Add extension support for Tor windows
4 participants
@darkdh @bridiver @petemill @mkarolin